feat(config): allow overriding the config path via URUNC_CONFIG_FILE - #989
feat(config): allow overriding the config path via URUNC_CONFIG_FILE#989Anand-240 wants to merge 1 commit into
Conversation
✅ Deploy Preview for urunc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hello @Anand-240 , testing through unit tests that the same PR adds is not enough. This should be tested end-to-end. Also, the LLM usage refers to the model used and not in the agent. The data generation (what we care) comes from the LLM. |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @Anand-240 ,
The code as code is good, the comments and documentation are not. I have added comments. When you are done you can make one of your open PRs drafts and mark this read for review.
81890c5 to
d309136
Compare
|
Thanks for the quick review @cmainas. Pushed the changes. I trimmed the ResolveUruncConfigPath comment so it ends at "otherwise", and cleaned up the docs line, dropped the second person and the Talos bit and kept one sentence with an example. Also fixed the LLM usage in the description to point to the model (Claude Opus 4.8) and not the agent. For the end to end testing, the e2e harness only sets guest env vars right now, not the runtime or shim env, so there isn't a clean way to pass URUNC_CONFIG_PATH into the shim from the test cases yet. Want me to extend the harness so a case can set the runtime env, or would you rather validate it another way? |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @Anand-240 ,
thank you for the updates. We should rename the environment variable to URUNC_CONFIG_FILE, since it is a file and not any path. Also, we should add a example.
Anand-240
changed the title
d309136 to
37da8f6
Compare
|
Thanks @cmainas. Renamed the env var to URUNC_CONFIG_FILE everywhere (code, tests and docs) and updated the title. Also added the docs example with the wrapper script approach. Let me know if you'd prefer it closer to your snippet. |
The config path was hardcoded to /etc/urunc/config.toml, which breaks on immutable or read-only hosts like Talos where /etc is not writable. Add ResolveUruncConfigPath, which reads the URUNC_CONFIG_FILE env var and falls back to /etc/urunc/config.toml when it is unset, and use it at the three places that load the config. Setting the var in the shim is enough since the urunc process it spawns inherits the environment. Fixes: urunc-dev#987 Signed-off-by: Anand-240 <anandprakashsrivastava68@gmail.com>
37da8f6 to
d57b36c
Compare
|
@cmainas I pushed a fix for the spell check, it was flagging "subtests" in a test comment so I reworded it. For the e2e failures, I had a look at the logs and they seem to fail during setup with "cannot verify s3.nbfc.io's certificate" (wget exit code 5) while pulling the test assets, before any container actually runs. Build and unit tests are green, and the change is a no-op when URUNC_CONFIG_FILE is unset since it resolves to the same /etc/urunc/config.toml as before, so I don't think it's coming from this PR, but please let me know if I'm reading it wrong. |
2 participants
Description
The config path was hardcoded to
/etc/urunc/config.toml, which does not work on immutable or read-only hosts like Talos where/etcis not writable. This adds aURUNC_CONFIG_PATHenv var to set the path, falling back to/etc/urunc/config.tomlwhen it is unset. Per #987 this is the temporary env var approach, before config handling moves to the shim.Related issues
How was this tested?
Added unit tests for the resolver (env set and unset) and ran golangci-lint locally with no issues.
LLM usage
The design, the discussion on #979 and #987, and the review were mine. Claude Opus 4.8 helped write the implementation, tests and docs.
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).